Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-30732: Allows table cells to contain inlines and blocks #56

Closed

Conversation

SerheyDolgushev
Copy link
Contributor

Question Answer
JIRA issue EZP-30732
Bug/Improvement yes
New feature no
Target version 1.1
BC breaks no
Tests pass yes
Doc needed no

Seems like there is a bug in doocbook.rng:

    <define name="db.html.td">
      <element name="td">
        <a:documentation>A table entry in an HTML table</a:documentation>
        <ref name="db.html.td.attlist"/>
        <choice>
          <zeroOrMore>
            <ref name="db.all.inlines"/>
          </zeroOrMore>
          <zeroOrMore>
            <ref name="db.all.blocks"/>
          </zeroOrMore>
        </choice>
      </element>
    </define>

The fist condition is always true:

          <zeroOrMore>
            <ref name="db.all.inlines"/>
          </zeroOrMore>

And thats if table cell cotnains any db.all.blocks - triggers a validation error:
Validation of XML content failed

The most correct fix would be to make this change in doocbook.rng.

This PR is related to ezsystems/ezplatform-admin-ui#1034.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

Copy link
Member

@vidarl vidarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SerheyDolgushev
Copy link
Contributor Author

What is the best way to contact docbook and let them know about this bug?

@ndw
Copy link

ndw commented Aug 12, 2019

Can you provide a sample document that you believe should validate but doesn't? This document validates for me:

<article xmlns="http://docbook.org/ns/docbook">
<title>Test</title>

<informaltable>
<tbody>
<tr>
  <th>Heading</th>
</tr>
<tr>
  <td><para>This is a paragraph.</para>
  </td>
</tr>
</tbody>
</informaltable>

</article>

@SerheyDolgushev
Copy link
Contributor Author

SerheyDolgushev commented Aug 12, 2019

@ndw can you try to test any block (not inline) element inside the table cell? F.e.

<article xmlns="http://docbook.org/ns/docbook">
<title>Test</title>

<informaltable>
<tbody>
<tr>
  <th>Heading</th>
</tr>
<tr>
  <td>
   <blockquote />
  </td>
</tr>
</tbody>
</informaltable>

</article>

a011b56 is faling without this PR.

@ndw
Copy link

ndw commented Aug 12, 2019

The problem with that example isn't in the table, it's in the blockquote. DocBook does not allow an empty block quotation. This validates just fine for me:

<article xmlns="http://docbook.org/ns/docbook">
<title>Test</title>

<informaltable>
<tbody>
<tr>
  <th>Heading</th>
</tr>
<tr>
  <td><blockquote><para/></blockquote>
  </td>
</tr>
</tbody>
</informaltable>

</article>

@SerheyDolgushev
Copy link
Contributor Author

@ndw you are correct. After spending more time on this issue, seems like the true reason was found: there was zero width space (U+200B) after the child element in the table cell. Child element was a block type. So our case was:

<article xmlns="http://docbook.org/ns/docbook">
  <title>Test</title>
  <informaltable>
    <tbody>
      <tr>
        <th>Heading</th>
      </tr>
      <tr>
        <td>
          <blockquote>
            <para/>
          </blockquote>
          [zero width space]
        </td>
      </tr>
    </tbody>
  </informaltable>
</article>

The current PR makes this content valid, and no validation erros are thrown. But, I assume, the more correct solution would be to fix the code which adds zero width space (instead of changing the rng schema).

@ndw can you think about any other cases where zero width space might lead to validation errors?

@SerheyDolgushev
Copy link
Contributor Author

@vidarl seems like this one might be closed in the flavor of ezsystems/ezplatform-admin-ui@9e8739d

@ndw
Copy link

ndw commented Aug 13, 2019 via email

@ndw
Copy link

ndw commented Aug 13, 2019 via email

@SerheyDolgushev
Copy link
Contributor Author

It took me a few minutes to get my head around this. The XML Recommendation defines whitespace as explicitly (and exclusively) SPACE ( ), TAB ( ), and NEWLINE ( )[]. Any other character that occurs between block elements (including zero-width space, non-breaking space, etc. etc. etc.) is effectively text where it isn't allowed. A zero-width space where only elements are allowed is going to cause validation errors every time. [] Technically, CARRIAGE RETURN (#xD;) is also part of the space production, but the parser removes them or converts them to NEWLINE in all cases except where they occur as explicit numeric character references.

On Tue, Aug 13, 2019 at 6:25 AM Serhey Dolgushev @.> wrote: @vidarl https://github.com/vidarl seems like this one might be closed in the flavor of @. <ezsystems/ezplatform-admin-ui@9e8739d> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56?email_source=notifications&email_token=AAAI7OKSJQVVKBZRPGFBJD3QEKKY5A5CNFSM4IKAGMB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4FLOFI#issuecomment-520795925>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAI7OMSLFTKIYW5VAB2GMTQEKKY5ANCNFSM4IKAGMBQ .

Yep, this one was fun :)
It was very difficult to discover that empty width space between the child elements of td. And as you said it counts like a text, which explains why validation error was triggered. So no changes in the schema are required. It needs to be fixed in the app logic which is responsible for generating XML.

@ndw Thank you a lot for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants